Skip to content

Allow install of static slang library#11458

Open
jkiviluoto-nv wants to merge 1 commit into
shader-slang:masterfrom
jkiviluoto-nv:fix-11359-static-install-export
Open

Allow install of static slang library#11458
jkiviluoto-nv wants to merge 1 commit into
shader-slang:masterfrom
jkiviluoto-nv:fix-11359-static-install-export

Conversation

@jkiviluoto-nv

Copy link
Copy Markdown
Contributor

Summary

A static-library install of Slang was blocked by an obsolete guard in CMakeLists.txt. The install(EXPORT SlangTargets) step and the UNIX pkgconfig install were skipped whenever SLANG_LIB_TYPE == STATIC.

The guard was added in #6158 to work around the export errors in #5821: slang's internal link dependencies (core, prelude, compiler-core, slang-capability-*, slang-lookup-tables, SPIRV-Headers, ...) leaked onto its public link interface and weren't in any export set, so install(EXPORT ...) failed.

That guard is now obsolete. slang_add_target wraps private link deps in $<BUILD_LOCAL_INTERFACE:...> / $<BUILD_INTERFACE:...> (cmake/SlangTarget.cmake), so those targets no longer appear in the install/export interface and a static install exports cleanly.

The guard also masked a second bug: configure_package_config_file runs unconditionally and the generated slangConfig.cmake unconditionally include()s slangTargets.cmake (cmake/SlangConfig.cmake.in). With the guard, a static install shipped a config that pointed at a Targets file that was never installed, so find_package(slang) failed with a confusing "file not found." Removing the guard fixes this too.

CI coverage

The CMake Options workflow already builds the SLANG_LIB_TYPE=STATIC entry, but only configured and built it and never ran cmake --install — so this regression would not have been caught. This PR adds an install_test matrix flag and a cmake --install step (in both the host and container build workflows) that runs for entries opting in via that flag.

Verification

Locally on macOS (CMake 4.3.3, SLANG_LIB_TYPE=STATIC):

  • Configure + generate complete with zero "not in any export set" diagnostics.
  • The exported slang target's INTERFACE_LINK_LIBRARIES is empty ($<LINK_ONLY:>) — no internal targets leak.
  • cmake --install succeeds and ships slangTargets.cmake, slang-compiler.pc, slangConfig.cmake, and slangConfigVersion.cmake.
  • A minimal find_package(slang CONFIG) against the install resolves the slang::slang target.

The original reporter independently confirmed guard-less static installs succeed on both MSVC and gcc.

Fixes #11359

The install(EXPORT SlangTargets) step and the UNIX pkgconfig install were
skipped whenever SLANG_LIB_TYPE was STATIC. The guard was added in shader-slang#6158 to
work around the export errors in shader-slang#5821: slang's internal link dependencies
(core, prelude, compiler-core, slang-capability-*, slang-lookup-tables,
SPIRV-Headers, ...) leaked onto its public link interface and weren't in any
export set, so install(EXPORT ...) failed.

That guard is now obsolete. slang_add_target wraps private link deps in
$<BUILD_LOCAL_INTERFACE:...> / $<BUILD_INTERFACE:...>, so those targets no
longer appear in the install/export interface and a static install exports
cleanly. The guard also masked a second bug: configure_package_config_file
runs unconditionally and the generated slangConfig.cmake unconditionally
include()s slangTargets.cmake, so a static install used to ship a config that
pointed at a Targets file that was never installed, making find_package(slang)
fail with a confusing "file not found".

Removing the guard fixes both: a static install now exports SlangTargets and
find_package(slang) resolves the slang::slang target.

Also add CI coverage: the CMake Options workflow already builds the
SLANG_LIB_TYPE=STATIC entry, but only configured and built it and never ran
cmake --install, so this regression would not have been caught. Add an
install_test matrix flag and a 'cmake --install' step (in both the host and
container build workflows) that runs for entries opting in via that flag.

Fixes shader-slang#11359
@jkiviluoto-nv jkiviluoto-nv requested a review from a team as a code owner June 3, 2026 18:15
@jkiviluoto-nv jkiviluoto-nv requested review from bmillsNV and removed request for a team June 3, 2026 18:15
@jkiviluoto-nv jkiviluoto-nv added the pr: non-breaking PRs without breaking changes label Jun 3, 2026
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR enables installation and CMake export of static library builds by removing a build-type condition that previously blocked install(EXPORT SlangTargets) for SLANG_LIB_TYPE=STATIC. The CMakeLists.txt changes unblock the export path for all library types and move pkg-config generation into the shared export block. CI validation is added via a new install_test matrix flag and corresponding workflow steps that exercise the install path when enabled.

Changes

Static Library Export and Installation

Layer / File(s) Summary
CMake export and install configuration
CMakeLists.txt
The install(EXPORT SlangTargets) block is now guarded only by CMAKE_SYSTEM_NAME != "Emscripten", removing the previous exclusion of SLANG_LIB_TYPE=STATIC. The UNIX pkg-config generation and installation logic is moved into the unconditional export block so it also runs for STATIC builds.
CI matrix and workflow integration for export testing
.github/cmake-options-matrix.json, .github/workflows/cmake-options-build-container.yml, .github/workflows/cmake-options-build.yml
The SLANG_LIB_TYPE=STATIC matrix entry is tagged with install_test: true, and both the container and standalone build workflows now conditionally run cmake --install steps that validate the export path for flagged configurations.

Suggested reviewers

  • bmillsNV
  • expipiplus1
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Allow install of static slang library' clearly and concisely describes the main change—removing the guard that previously blocked static library installation.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, explaining the obsolete guard removal, the underlying fix via slang_add_target, the second bug it masked, and CI coverage additions.
Linked Issues check ✅ Passed The PR fully addresses issue #11359 by removing the CMake condition that blocked static library installation and adding CI verification to catch regressions.
Out of Scope Changes check ✅ Passed All changes are within scope: CMakeLists.txt removes the obsolete STATIC guard, workflow files add install_test verification, and cmake-options-matrix.json flags SLANG_LIB_TYPE=STATIC for install testing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot requested a review from expipiplus1 June 3, 2026 18:17

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: dba2233e-0bec-4ceb-ac59-8151b2518d11

📥 Commits

Reviewing files that changed from the base of the PR and between bbc4b02 and 4d1979f.

📒 Files selected for processing (4)
  • .github/cmake-options-matrix.json
  • .github/workflows/cmake-options-build-container.yml
  • .github/workflows/cmake-options-build.yml
  • CMakeLists.txt

Comment on lines +150 to +156
# Exercise `cmake --install` for entries that opt in via install_test.
# The static-lib build (SLANG_LIB_TYPE=STATIC) used to skip the
# install(EXPORT ...) step entirely, so a broken export would not have
# been caught by a plain configure+build. See issue #11359.
- name: Install Slang (${{ matrix.option }})
if: matrix.install_test == 'true' && matrix.windows_only != 'true'
run: cmake --install build --config "$cmake_config" --prefix install

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift

Consider validating find_package(slang) after install, not just that cmake --install succeeds.

Issue #11359 manifested as find_package(slang) failing because slangTargets.cmake wasn't installed — yet the install command itself still succeeded with the old guard in place. The "not in any export set" failure is caught at configure/generate time now, but this step alone would not catch a future regression where the install completes but the exported package config is inconsistent. A minimal consumer find_package(slang CONFIG PATHS install ... NO_DEFAULT_PATH) check (resolving slang::slang) would directly guard the originally-reported breakage.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 🟡 Has issues — 2 gap(s)

Removes an obsolete if(NOT ${SLANG_LIB_TYPE} STREQUAL "STATIC") guard around install(EXPORT SlangTargets ...) and the UNIX pkgconfig install in CMakeLists.txt, fixing the static-install / find_package(slang) failure tracked in #11359, and adds an install_test matrix flag with a cmake --install step. Two gaps: the pkgconfig file is now installed for STATIC builds even though its template explicitly does not support static linking and lacks Libs.private; and the new cmake --install step doesn't exercise the find_package(slang) consumer path that originally broke.

Changes Overview

CMake export gating (CMakeLists.txt)

  • What changed: removes the if(NOT ${SLANG_LIB_TYPE} STREQUAL "STATIC") wrapper around both install(EXPORT SlangTargets ...) and the UNIX slang-compiler.pc install. Adds a comment block documenting why the guard (originally added in #6158) is now obsolete: slang_add_target wraps private link deps in $<BUILD_LOCAL_INTERFACE:...> / $<BUILD_INTERFACE:...>, so internal targets no longer leak onto the public export interface. This is verifiable in cmake/SlangTarget.cmake (around lines 434-443) and cmake/SlangConfig.cmake.in.

Install regression coverage (.github/cmake-options-matrix.json, .github/workflows/cmake-options-build.yml, .github/workflows/cmake-options-build-container.yml)

  • What changed: introduces a new install_test matrix flag, set on the SLANG_LIB_TYPE=STATIC entry, and an "Install Slang" step in both the host and container cmake-options workflows that runs cmake --install build --config "$cmake_config" --prefix install for entries opting in.
Findings (2 total)
Severity Location Finding
🟡 Gap CMakeLists.txt:694 pkgconfig file is now installed for STATIC builds, but the .pc.in template doesn't populate Libs.private and explicitly says static linking is unsupported
🟡 Gap .github/workflows/cmake-options-build.yml:187 New cmake --install step doesn't exercise the find_package(slang) consumer path that originally broke in #11359

Comment thread CMakeLists.txt
endif()
# TODO: When build system is changed to respect CMAKE_INSTALL_LIBDIR,
# update this destination.
install(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Gap: pkgconfig file installed for STATIC builds, but .pc template explicitly does not support static linking

Removing the STATIC guard makes install(FILES "${PROJECT_BINARY_DIR}/slang-compiler.pc" DESTINATION "lib/pkgconfig") fire for UNIX static builds too. extras/pkgconfig/slang-compiler.pc.in is not equipped for that case:

Libs: -L${libdir} -lslang-compiler
Cflags: -I${includedir}

# Note: pkg-config --static is not currently supported,
# so Libs.private is not populated.

Libs: names only -lslang-compiler and Libs.private is empty. After a STATIC install, pkg-config --libs slang-compiler returns an incomplete link line that omits every transitive internal archive (core, compiler-core, slang-capability-*, slang-lookup-tables, plus bundled vendored libs like miniz/lz4 — see docs/building.md listing the static dependency chain). pkg-config users following the manifest will hit unresolved-symbol errors at link time.

Example: a downstream UNIX project does pkg-config --libs slang-compiler after cmake --install of a SLANG_LIB_TYPE=STATIC Slang. They get -L<prefix>/lib -lslang-compiler and ld reports unresolved references into core::* and compiler-core::*. Before this PR, no .pc was installed so the project would fall back to find_package(slang) (which is the well-supported path with this PR).

Suggested fix: either keep the pkgconfig install gated on shared (the simplest patch and consistent with the .pc.in comment), or populate Libs.private with the full static dependency chain when SLANG_LIB_TYPE is STATIC and document that pkg-config --static --libs slang-compiler is required.

if(UNIX AND NOT ${SLANG_LIB_TYPE} STREQUAL "STATIC")
    configure_file(...)
    install(FILES "${PROJECT_BINARY_DIR}/slang-compiler.pc" DESTINATION "lib/pkgconfig")
endif()

# been caught by a plain configure+build. See issue #11359.
- name: Install Slang (${{ matrix.option }})
if: matrix.install_test == 'true' && (matrix.linux_only != 'true' || inputs.os == 'linux') && (matrix.windows_only != 'true' || inputs.os == 'windows') && matrix.container_only != 'true' && (matrix.skip_linux_aarch64 != 'true' || inputs.os != 'linux' || inputs.platform != 'aarch64')
run: cmake --install build --config "$cmake_config" --prefix install

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Gap: cmake --install doesn't exercise the consumer-side regression of #11359

The reported failure mode in #11359 is find_package(slang) blowing up on a static install because slangConfig.cmake always include()s a slangTargets.cmake that wasn't installed. The new step only runs cmake --install build --config "$cmake_config" --prefix install. That covers the case where install(EXPORT SlangTargets ...) itself errors at install time, but it doesn't exercise the actual user path that broke.

In particular, if a future change re-introduces a guard around just the install(EXPORT ...) call (the exact shape of the original bug), cmake --install still succeeds — it just silently doesn't ship slangTargets.cmake. The regression resurfaces only when a downstream calls find_package(slang), which CI never does.

Suggested fix: chain a tiny find_package smoke test after the install step. The slang_DIR path differs by OS because SLANG_CMAKE_CONFIG_DIR is cmake on Windows and ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME} elsewhere (CMakeLists.txt around line 652).

- name: Smoke-test find_package(slang) against install prefix
  if: matrix.install_test == 'true' && (matrix.linux_only != 'true' || inputs.os == 'linux') && (matrix.windows_only != 'true' || inputs.os == 'windows') && matrix.container_only != 'true' && (matrix.skip_linux_aarch64 != 'true' || inputs.os != 'linux' || inputs.platform != 'aarch64')
  shell: bash
  run: |
    mkdir -p find_pkg_test && cd find_pkg_test
    cat > CMakeLists.txt <<'EOF'
    cmake_minimum_required(VERSION 3.22)
    project(find_slang_smoke LANGUAGES CXX)
    find_package(slang CONFIG REQUIRED)
    if(NOT TARGET slang::slang)
      message(FATAL_ERROR "slang::slang target missing after find_package")
    endif()
    EOF
    cmake -S . -B build -Dslang_ROOT="$GITHUB_WORKSPACE/install"

Same change applies to .github/workflows/cmake-options-build-container.yml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: non-breaking PRs without breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Strange cmake condition that blocks install static library

1 participant